Skip to content

fix(client): preserve canonical format options in get_products - #1006

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-1005-preserve-canonical-products
Aug 5, 2026
Merged

fix(client): preserve canonical format options in get_products#1006
bokelley merged 1 commit into
mainfrom
claude/issue-1005-preserve-canonical-products

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Closes #1005.

What changed

  • Parse get_products responses through the canonical GetProductsResponse model first.
  • Preserve the existing legacy response projection as a fallback when canonical validation fails.
  • Cache canonical product routes and attach an empty projection-diagnostics envelope for direct canonical responses.
  • Add regression coverage for canonical-only products and for the legacy format_ids fallback.

Root cause

_canonicalize_get_products_result() parsed every response through LegacyGetProductsResponse before projection. The generated legacy ProductFormatDeclaration does not declare format_kind or params, so Pydantic discarded those canonical fields before project_legacy_product() saw them. Valid canonical-only products were then omitted with FORMAT_PROJECTION_FAILED diagnostics.

Impact

Python buyers using the primary ADCPClient.get_products() API now retain canonical format_options. Legacy-only sellers continue through the compatibility projector.

Validation

  • make test: 6,200 passed, 41 skipped, 1 xfailed; 84.18% coverage
  • make typecheck-all: passed (932 SDK source files plus strict adopter fixtures)
  • make lint: passed
  • File-scoped pre-commit suite: passed, including Bandit
  • Live regression against Embedded Sales Agent:
    • released v7.0.0-rc: 2 wire products became 0 with four projection errors
    • this branch: both products and canonical image declarations were preserved

@bokelley
bokelley marked this pull request as ready for review August 5, 2026 02:27
@bokelley
bokelley enabled auto-merge (squash) August 5, 2026 02:27

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. Right shape: the canonical GetProductsResponse is the primary read surface, so it must parse before the legacy compatibility model — not after it has already silently stripped format_kind/params.

Real bug. The old _canonicalize_get_products_result routed every response through LegacyGetProductsResponse first, whose generated ProductFormatDeclaration lacks the canonical fields, so canonical-only products were discarded and re-emitted as FORMAT_PROJECTION_FAILED. Live regression in the PR body tells the story: v7.0.0-rc turned 2 wire products into 0 with four projection errors; this branch preserves both plus their image declarations.

Things I checked

  • No fall-through regression for legacy format_ids. The public GetProductsResponse (src/adcp/types/_eager.py:732canonical_creative.py:631) inherits _reject_legacy_creative_identity, a mode="before" validator that raises on format_id/format_ids/v1_format_ref at any depth (canonical_creative.py:323). So a legacy-only or dual-emission product fails whole-response canonical validation deterministically → canonical_result.success is False → falls through to project_legacy_product. test_get_products_still_projects_legacy_format_ids passes because canonical parse fails, which is the intended mechanism, not a coincidence.
  • No mixed-response data loss. Whole-response model_validate is atomic — one legacy product routes the entire set through projection. format_ids is never silently stripped on input; presence raises. ad-tech-protocol-expert: sound — "the failure mode you'd worry about is structurally impossible."
  • Control flow at client.py:1514. Checking raw_result.success instead of legacy_result.success is equivalent: _parse_response short-circuits on a failed/empty raw result before parsing (protocols/base.py:114), so canonical_result in that branch is byte-for-byte the old legacy_result return.
  • In-place canonical_result.metadata = metadata (client.py:1521) is safeTaskResult is not frozen and has no validate_assignment; the object is freshly built by _parse_response.
  • Diagnostics envelope parity. {"diagnostics": []} on the direct path is accurate — a fully canonical response required zero projection. No diagnostics are lost: any malformed format_options entry fails canonical parse and the legacy path emits FORMAT_PROJECTION_FAILED as before.
  • Semver signal. Private helper; public get_products() behavior only restores dropped data. fix(client): is correct — no ! needed.
  • Test plan: make test 6,200 passed, make typecheck-all passed, make lint passed, Bandit passed. code-reviewer: no blockers. ad-tech-protocol-expert: sound.

Follow-ups (non-blocking — file as issues)

  • Response-level all-or-nothing fallback. One legacy or malformed product in an otherwise-canonical response routes all products through projection. Functionally a superset handler, but a clean product's direct-vs-projection path now depends on its siblings. Worth a comment near client.py:1508 documenting that the fast path is reachable only for fully-canonical responses.

Minor nits (non-blocking)

  1. Double parse on the legacy path. client.py:1509 now runs a full canonical parse that always fails before the legacy parse for legacy-only sellers. Negligible, but notable for high-QPS wholesale-feed enumeration.

Approving on the strength of the structural fall-through guarantee plus the live regression.

@bokelley
bokelley merged commit 96396b9 into main Aug 5, 2026
27 checks passed
@bokelley
bokelley deleted the claude/issue-1005-preserve-canonical-products branch August 5, 2026 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(client): preserve canonical format options in get_products

1 participant